Move unique functionality into getGroups to reduce calls to google #2628

snuggie12 · 2022-08-12T23:26:21Z

Overview

Removed the unique function and integrated into getGroups to reduce the number of calls to google.

What this PR does / why we need it

Depending on the groups a user belongs to the current code can make several extraneous calls to google. This can occur either when a user has several indirect memberships to a group or when the user has both an indirect and direct membership to a group:

# two extraneous calls to numbers
# email of user/group -> [groups email is a member of]
user -> [one, two, three]
one -> [numbers]
two -> [numbers]
three -> [numbers]
numbers -> []

# extraneous call to bar due to both direct and indirect membership
user -> [foo, bar]
foo -> [bar]
bar -> []

Implementing a standard map setup reduces calls to google down to number of groups + original user.

I'm not sure if google allows it, but it also eliminates a stack overflow if there was a circular membership:

user -> [one]
one -> [two]
two -> [three]
three -> [one]

Special notes for your reviewer

Does this PR introduce a user-facing change?

NONE

snuggie12 · 2022-08-15T17:54:01Z

@nabokihms do I add a label or do you all? It wasn't mentioned in the PR template, but wasn't sure if you would get notified of the PR until it's there

nabokihms

Thanks for opening this PR. I left some questions. Could you please have a look?

P.S. Don't forget to sign your commits.

nabokihms · 2022-08-17T14:56:31Z

connector/google/google.go

@@ -257,12 +258,15 @@ func (c *googleConnector) getGroups(email string, fetchTransitiveGroupMembership
 		}

 		for _, group := range groupsList.Groups {
-			// TODO (joelspeed): Make desired group key configurable
-			userGroups = append(userGroups, group.Email)
+			if _, exists := (*checkedGroups)[group.Email]; !exists {


Is it possible to add a test that the number of API calls is reduced?

Yeah, I would like to add a test. Would I need to setup a mock for that? Is there an example?

Locally I have logs which demonstrate this. I could add that to the code instead.

Oh, there are no tests for Google connector at all...

You can use this code as an example to write a simple test.

The example looked easy enough in that you could send to whatever URL you want but the google connector is using a library and so I couldn't figure out how to do it in a similar fashion.

I attempted both interfaces and embedded structs but couldn't get around the issue of getGroups always calling its version of getGroupsList. In the end I settled on getGroups taking a function as a parameter and that worked.

Hopefully that is okay, because otherwise I fear I might need a lot of hand holding to get something working (not a developer by trade.)

@snuggie12, I've implemented some unit testing for your PR on my fork: https://github.com/ichbinfrog/dex/blob/reduce-admin-api-calls/connector/google/google_test.go.

Feel free to pull / take inspiration from the test file and reuse it since it also adds the required instrumentation for the amount of apiCalls.

TLDR: the basic gist of mocking google APIs is to inject a few client options which will force it to point to a local http server whose behavior you can control:

conn.adminSrv, err = admin.NewService( context.Background(), option.WithoutAuthentication(), option.WithEndpoint(ts.URL), )

@snuggie12, I've implemented some unit testing for your PR on my fork: https://github.com/ichbinfrog/dex/blob/reduce-admin-api-calls/connector/google/google_test.go.

Feel free to pull / take inspiration from the test file and reuse it since it also adds the required instrumentation for the amount of apiCalls.

TLDR: the basic gist of mocking google APIs is to inject a few client options which will force it to point to a local http server whose behavior you can control:

conn.adminSrv, err = admin.NewService( context.Background(), option.WithoutAuthentication(), option.WithEndpoint(ts.URL), )

@ichbinfrog thanks for this. I've been busy moving where I live but I'll def take a look soon.

connector/google/google.go

nabokihms · 2022-08-20T04:10:00Z

connector/google/google.go

-				transitiveGroups, err := c.getGroups(group.Email, fetchTransitiveGroupMembership)
-				if err != nil {
-					return nil, fmt.Errorf("could not list transitive groups: %v", err)
+			if _, exists := checkedGroups[group.Email]; !exists {


NIT: I'd like to reduce the indentation level here, e.g.,

if _, exists := checkedGroups[group.Email]; exists { continue } ... if !fetchTransitiveGroupMembership { continue } ...

Nested conditions wrapped in two for loops are a little bit harder to read.

Perhaps use some tools in the build pipeline that do static analysis of cognitive and cyclomatic complexity.

nabokihms · 2022-08-20T04:12:00Z

connector/google/google.go

@@ -257,12 +258,15 @@ func (c *googleConnector) getGroups(email string, fetchTransitiveGroupMembership
 		}

 		for _, group := range groupsList.Groups {
-			// TODO (joelspeed): Make desired group key configurable
-			userGroups = append(userGroups, group.Email)
+			if _, exists := (*checkedGroups)[group.Email]; !exists {


Oh, there are no tests for Google connector at all...

You can use this code as an example to write a simple test.

nabokihms

Looks nice! Things that stop us from merging this PR:

A test.
DCO.

snuggie12 · 2022-08-23T20:33:26Z

Looks nice! Things that stop us from merging this PR:
1. A test.

2. [DCO](https://github.com/apps/dco).

I'll get on adding the test now. Had a production incident all day yesterday to handle but should be able to get to it the next couple of days.

For DCO, is that just you want the initial commit to have the Signed-off-by line like the 2nd commit currently has or is there something more to it that I missed from your link?

Do you mind if I add a few debug level log lines stating which email is being checked and later if the group exists? If yes, do you want me to use else or log and still use continue??

snuggie12 · 2022-09-14T21:41:57Z

@ichbinfrog I reverted my google.go prior to me adding tests where I changed how some methods worked. After a mistake in merging master into my branch the tests now pass locally

snuggie12 · 2022-10-03T20:23:14Z

@nabokihms I don't see a way to restart that one job and it appears to be unrelated to my change. Can you restart or merge anyways?

snuggie12 · 2022-10-10T17:56:15Z

@nabokihms was there anything else you needed? I'd like to get this merged as soon as possible as I think I've seen some security releases and my company is currently running a custom build to fix this problem.

snuggie12 · 2022-10-13T16:32:25Z

@sagikazarmark sorry if you're the wrong person. Saw you have lots of commits and modified my milestone. Who should I ask for review on this? Really need this feature and AFAIK all things are signed-off.

Signed-off-by: Matt Hoey <matt.hoey@missionlane.com>

nabokihms · 2022-12-22T23:45:23Z

@snuggie12 sorry for the delay, and thank you for all your work and patience.

@xtremerui

The official container image for this release can be pulled from ``` ghcr.io/dexidp/dex:v2.36.0 ```  ## What's Changed ### Enhancements 🚀 * TLS configure for OIDC connector by @xtremerui in dexidp#1632 * Add icon for gitea by @pinpox in dexidp#2733 * fix: Do not use connector data from the refresh token field by @nabokihms in dexidp#2729 * Add preferredEmailDomain config option for GitHub connector by @nobuyo in dexidp#2740 * Move unique functionality into getGroups to reduce calls to google by @snuggie12 in dexidp#2628 * fix: prevent server-side request forgery using Kubernetes storage by @nabokihms in dexidp#2479 * fix: return 401 if password is invalid by @nabokihms in dexidp#2796 * feat: Add default robots.txt by @nabokihms in dexidp#2834 * Skip redirection to approval when it is not required (dexidp#2686) by @nobuyo in dexidp#2805 * feat: Bump dependencies and Makefile refactoring by @nabokihms in dexidp#2844 ### Bug Fixes 🐛 * Make admin email optional when no service account path is configured by @sagikazarmark in dexidp#2695 * Only initialize google admin service if necessary by @sagikazarmark in dexidp#2700 ### Dependency Updates ⬆️ * build(deps): bump golang from 1.19.1-alpine3.16 to 1.19.2-alpine3.16 by @dependabot in dexidp#2697 * fix: Update gomplate version to 3.11.3 fix CVE-2022-27665 by @nabokihms in dexidp#2705 * build(deps): bump github.com/spf13/cobra from 1.5.0 to 1.6.0 by @dependabot in dexidp#2708 * build(deps): bump github.com/stretchr/testify from 1.8.0 to 1.8.1 by @dependabot in dexidp#2715 * build(deps): bump google.golang.org/api from 0.98.0 to 0.101.0 by @dependabot in dexidp#2720 * build(deps): bump github.com/mattn/go-sqlite3 from 1.14.15 to 1.14.16 by @dependabot in dexidp#2721 * build(deps): bump aquasecurity/trivy-action from 0.7.1 to 0.8.0 by @dependabot in dexidp#2723 * build(deps): bump github.com/spf13/cobra from 1.6.0 to 1.6.1 by @dependabot in dexidp#2718 * build(deps): bump golang from 1.19.2-alpine3.16 to 1.19.3-alpine3.16 by @dependabot in dexidp#2724 * build(deps): bump alpine from 3.16.2 to 3.17.0 by @dependabot in dexidp#2746 * build(deps): bump github.com/prometheus/client_golang from 1.13.0 to 1.14.0 by @dependabot in dexidp#2735 * build(deps): bump go.etcd.io/etcd/client/pkg/v3 from 3.5.5 to 3.5.6 by @dependabot in dexidp#2744 * build(deps): bump github.com/Masterminds/sprig/v3 from 3.2.2 to 3.2.3 by @dependabot in dexidp#2751 * build(deps): bump golang from 1.19.3-alpine3.16 to 1.19.4-alpine3.16 by @dependabot in dexidp#2750 * build(deps): bump golang.org/x/crypto from 0.3.0 to 0.4.0 by @dependabot in dexidp#2755 * build(deps): bump go.etcd.io/etcd/client/v3 from 3.5.5 to 3.5.6 by @dependabot in dexidp#2743 * build(deps): bump github.com/go-sql-driver/mysql from 1.6.0 to 1.7.0 by @dependabot in dexidp#2754 * build(deps): bump helm/kind-action from 1.4.0 to 1.5.0 by @dependabot in dexidp#2758 * build(deps): bump google.golang.org/grpc from 1.50.1 to 1.51.0 by @dependabot in dexidp#2741 * build(deps): bump google.golang.org/api from 0.101.0 to 0.104.0 by @dependabot in dexidp#2753 * build(deps): bump google.golang.org/grpc from 1.49.0 to 1.51.0 in /api/v2 by @dependabot in dexidp#2742 * build(deps): bump golang.org/x/net from 0.3.0 to 0.4.0 by @dependabot in dexidp#2761 * build(deps): bump entgo.io/ent from 0.11.3 to 0.11.4 by @dependabot in dexidp#2725 * build(deps): bump google.golang.org/api from 0.104.0 to 0.105.0 by @dependabot in dexidp#2760 * build(deps): bump golang.org/x/net from 0.4.0 to 0.5.0 by @dependabot in dexidp#2774 * build(deps): bump google.golang.org/api from 0.105.0 to 0.106.0 by @dependabot in dexidp#2772 * build(deps): bump github.com/coreos/go-oidc/v3 from 3.4.0 to 3.5.0 by @dependabot in dexidp#2770 * build(deps): bump golang.org/x/crypto from 0.4.0 to 0.5.0 by @dependabot in dexidp#2773 * build(deps): bump golang.org/x/oauth2 from 0.3.0 to 0.4.0 by @dependabot in dexidp#2777 * build(deps): bump entgo.io/ent from 0.11.4 to 0.11.5 by @dependabot in dexidp#2779 * build(deps): bump alpine from 3.17.0 to 3.17.1 by @dependabot in dexidp#2780 * build(deps): bump mheap/github-action-required-labels from 2 to 3 by @dependabot in dexidp#2769 * build(deps): bump google.golang.org/api from 0.106.0 to 0.107.0 by @dependabot in dexidp#2788 * build(deps): bump golang from 1.19.4-alpine3.16 to 1.19.5-alpine3.16 by @dependabot in dexidp#2782 * build(deps): bump google.golang.org/grpc from 1.51.0 to 1.52.0 by @dependabot in dexidp#2783 * build(deps): bump google.golang.org/api from 0.107.0 to 0.108.0 by @dependabot in dexidp#2793 * build(deps): bump google.golang.org/grpc from 1.51.0 to 1.52.0 in /api/v2 by @dependabot in dexidp#2784 * chore: Upgrade golangci-lint to v1.50.1 from v1.46.0 by @dlipovetsky in dexidp#2790 * ci: Use go 1.19 by @dlipovetsky in dexidp#2791 * build(deps): bump go.etcd.io/etcd/client/v3 from 3.5.6 to 3.5.7 by @dependabot in dexidp#2798 * build(deps): bump docker/build-push-action from 3 to 4 by @dependabot in dexidp#2807 * build(deps): bump golang from 1.19.5-alpine3.16 to 1.20.0-alpine3.16 by @dependabot in dexidp#2811 * build(deps): bump aquasecurity/trivy-action from 0.8.0 to 0.9.0 by @dependabot in dexidp#2810 * build(deps): bump alpine from 3.17.1 to 3.17.2 by @dependabot in dexidp#2821 * build(deps): bump aquasecurity/trivy-action from 0.9.0 to 0.9.1 by @dependabot in dexidp#2822 * build(deps): bump entgo.io/ent from 0.11.5 to 0.11.8 by @dependabot in dexidp#2823 * build(deps): bump golang.org/x/crypto from 0.5.0 to 0.6.0 by @dependabot in dexidp#2818 * build(deps): bump golang.org/x/net from 0.5.0 to 0.7.0 by @dependabot in dexidp#2828 * build(deps): bump golang.org/x/net from 0.4.0 to 0.7.0 in /api/v2 by @dependabot in dexidp#2832 * build(deps): bump golang.org/x/sys from 0.0.0-20220114195835-da31bd327af9 to 0.1.0 in /examples by @dependabot in dexidp#2837 * build(deps): bump golang.org/x/net from 0.0.0-20220114011407-0dd24b26b47d to 0.7.0 in /examples by @dependabot in dexidp#2846 * build(deps): bump golang from 1.20.0-alpine3.16 to 1.20.1-alpine3.16 by @dependabot in dexidp#2827 * build(deps): bump aquasecurity/trivy-action from 0.9.1 to 0.9.2 by @dependabot in dexidp#2850 * build(deps): bump golang from 1.20.1-alpine3.16 to 1.20.2-alpine3.16 by @dependabot in dexidp#2849 * feat: Bump gomplate 3.11.4 by @nabokihms in dexidp#2840 * build(deps): bump golang.org/x/crypto from 0.6.0 to 0.7.0 by @dependabot in dexidp#2856 * build(deps): bump golang.org/x/oauth2 from 0.4.0 to 0.6.0 by @dependabot in dexidp#2847 * build(deps): bump google.golang.org/api from 0.108.0 to 0.112.0 by @dependabot in dexidp#2853 * build(deps): bump google.golang.org/api from 0.112.0 to 0.114.0 by @dependabot in dexidp#2869 * build(deps): bump actions/setup-go from 3 to 4 by @dependabot in dexidp#2863 * build(deps): bump github.com/russellhaering/goxmldsig from 1.2.0 to 1.3.0 by @dependabot in dexidp#2862 * build(deps): bump google.golang.org/protobuf from 1.28.1 to 1.30.0 by @dependabot in dexidp#2866 * build(deps): bump google.golang.org/protobuf from 1.28.1 to 1.30.0 in /api/v2 by @dependabot in dexidp#2867 * build(deps): bump golang.org/x/crypto from 0.0.0-20220112180741-5e0467b6c7ce to 0.1.0 in /examples by @dependabot in dexidp#2845 * build(deps): bump google.golang.org/grpc from 1.52.0 to 1.53.0 in /api/v2 by @dependabot in dexidp#2816 * chore: upgrade tools by @sagikazarmark in dexidp#2870 ### Other Changes * Bump image in examples/k8s/dex.yaml to v2.32.0 by @stealthybox in dexidp#2569 ## New Contributors * @pinpox made their first contribution in dexidp#2733 * @nobuyo made their first contribution in dexidp#2740 * @dlipovetsky made their first contribution in dexidp#2790 * @seankhliao made their first contribution in dexidp#2812 * @stealthybox made their first contribution in dexidp#2569 **Full Changelog**: dexidp/dex@v2.35.3...v2.36.0

…exidp#2628) Signed-off-by: Matt Hoey <matt.hoey@missionlane.com>

nabokihms added release-note/enhancement Release note: Enhancements area/connector labels Aug 15, 2022

nabokihms self-requested a review August 15, 2022 18:27

nabokihms added this to the v2.34.0 milestone Aug 15, 2022

nabokihms requested changes Aug 17, 2022

View reviewed changes

nabokihms self-requested a review August 18, 2022 06:07

nabokihms reviewed Aug 20, 2022

View reviewed changes

nabokihms requested changes Aug 20, 2022

View reviewed changes

snuggie12 force-pushed the reduced-transitive-calls branch from 3328d30 to f8409dd Compare September 14, 2022 21:39

snuggie12 force-pushed the reduced-transitive-calls branch 2 times, most recently from 5cde59f to 093f73b Compare September 14, 2022 22:12

nabokihms modified the milestones: v2.34.0, v2.35.0 Sep 16, 2022

sagikazarmark modified the milestones: v2.35.0, v2.36.0 Sep 29, 2022

snuggie12 force-pushed the reduced-transitive-calls branch 2 times, most recently from 94367c7 to 9950855 Compare October 3, 2022 19:17

snuggie12 force-pushed the reduced-transitive-calls branch from 9950855 to 6b6fef4 Compare October 28, 2022 21:52

Move unique functionality into getGroups to reduce calls to google

1c544f5

Signed-off-by: Matt Hoey <matt.hoey@missionlane.com>

snuggie12 force-pushed the reduced-transitive-calls branch from 6b6fef4 to 1c544f5 Compare October 29, 2022 18:55

nabokihms self-requested a review November 1, 2022 18:56

nabokihms approved these changes Dec 22, 2022

View reviewed changes

nabokihms merged commit c167276 into dexidp:master Dec 22, 2022

palexster pushed a commit to palexster/dex that referenced this pull request Oct 4, 2023

Move unique functionality into getGroups to reduce calls to google (d…

1fae51e

…exidp#2628) Signed-off-by: Matt Hoey <matt.hoey@missionlane.com>

michaelliau pushed a commit to FlockFreight/dex that referenced this pull request Oct 4, 2023

Move unique functionality into getGroups to reduce calls to google (d…

f7af8ca

…exidp#2628) Signed-off-by: Matt Hoey <matt.hoey@missionlane.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move unique functionality into getGroups to reduce calls to google #2628

Move unique functionality into getGroups to reduce calls to google #2628

snuggie12 commented Aug 12, 2022

snuggie12 commented Aug 15, 2022

nabokihms left a comment •

edited

Loading

nabokihms Aug 17, 2022

snuggie12 Aug 17, 2022

nabokihms Aug 20, 2022

snuggie12 Aug 25, 2022

ichbinfrog Sep 10, 2022

snuggie12 Sep 13, 2022

nabokihms Aug 20, 2022

unaryops Aug 25, 2022

nabokihms Aug 20, 2022

nabokihms left a comment

snuggie12 commented Aug 23, 2022

snuggie12 commented Sep 14, 2022 •

edited

Loading

snuggie12 commented Oct 3, 2022

snuggie12 commented Oct 10, 2022

snuggie12 commented Oct 13, 2022

nabokihms commented Dec 22, 2022

Move unique functionality into getGroups to reduce calls to google #2628

Move unique functionality into getGroups to reduce calls to google #2628

Conversation

snuggie12 commented Aug 12, 2022

Overview

What this PR does / why we need it

Special notes for your reviewer

Does this PR introduce a user-facing change?

snuggie12 commented Aug 15, 2022

nabokihms left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabokihms left a comment

Choose a reason for hiding this comment

snuggie12 commented Aug 23, 2022

snuggie12 commented Sep 14, 2022 • edited Loading

snuggie12 commented Oct 3, 2022

snuggie12 commented Oct 10, 2022

snuggie12 commented Oct 13, 2022

nabokihms commented Dec 22, 2022

nabokihms left a comment •

edited

Loading

snuggie12 commented Sep 14, 2022 •

edited

Loading